Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Port some commits from from joyent/node@master #1770

Merged
merged 2 commits into from
Jun 1, 2015
Merged

Port some commits from from joyent/node@master #1770

merged 2 commits into from
Jun 1, 2015

Conversation

Fishrock123
Copy link
Contributor

Ports a couple of commits over from joyent/node (master) that would be useful here.

Refs: #17, #21, #31

7532d24 repl: Private Buffer object in lib/* files should probably have a test of some sort, or we should somehow check that removing the Buffer global doesn't cause issues in core somewhere else.

Chris suggested running Buffer = {} though a repl instance, but I'm pretty sure that would only catch the readline module without extensive use of other modules.

Generally, I'm still leaning towards making global.Buffer non-writable.

@domenic
Copy link
Contributor

domenic commented May 22, 2015

Generally, I'm still leaning towards making global.Buffer non-writable.

Mehhhh, that's so un-JavaScriptey. The repl commit is the better approach.

We could make the linter disallow using Buffer as a global.

@Fishrock123
Copy link
Contributor Author

We could make the linter disallow using Buffer as a global.

👍 Not exactly sure how though. :P

Maybe cc @silverwind?

@mscdex mscdex added the meta Issues and PRs related to the general management of the project. label May 22, 2015
@silverwind
Copy link
Contributor

You could disable the Buffer global (and all other predefined ones) by removing this here:

https://github.com/nodejs/io.js/blob/9da168b71fb729635ad71e839630480e815623d0/.eslintrc#L2

Afterwards, specify all globals manually here:

https://github.com/nodejs/io.js/blob/9da168b71fb729635ad71e839630480e815623d0/.eslintrc#L78

If you want something readonly add it as false.

@bnoordhuis
Copy link
Member

I agree with the general thrust of 7532d24 but it isn't comprehensive enough, it only updates some of the files in lib/.

$ git grep -lw Buffer lib/
lib/_debug_agent.js  # this the only one that does require('buffer').Buffer
lib/_debugger.js
lib/_http_client.js
lib/_http_outgoing.js
lib/_stream_readable.js
lib/_stream_writable.js
lib/_tls_legacy.js
lib/_tls_wrap.js
lib/assert.js
lib/buffer.js
lib/child_process.js
lib/crypto.js
lib/dgram.js
lib/fs.js
lib/internal/smalloc.js
lib/net.js
lib/querystring.js
lib/readline.js
lib/string_decoder.js
lib/tls.js
lib/util.js
lib/zlib.js

@Fishrock123
Copy link
Contributor Author

@silverwind that didn't seem to work for Buffer. Also turning off node throws some peculiar errors about returning after a process.exit()..

@silverwind
Copy link
Contributor

@Fishrock123 I think that's stuff for another PR. When disabling node you need to specify globalReturn. And to really make it error on global write access, we need to enable no-undef, which results in a lot of linting failures in test, which need to be investigated first.

@Fishrock123
Copy link
Contributor Author

Ok, removed that commit since it's not comprehensive and we don't have a good way to keep linting it yet.

@silverwind silverwind mentioned this pull request May 25, 2015
Closed
@jasnell jasnell mentioned this pull request May 27, 2015
Closed
@Fishrock123
Copy link
Contributor Author

@silverwind mind signing off on the remaining two commits here?

@silverwind
Copy link
Contributor

LGTM

On a sidenote, I noticed doc/template.html refers to assets/sh.css while the directory is actually called api_assets.

@Fishrock123
Copy link
Contributor Author

Weird. Noted.

robertkowalski and others added 2 commits June 1, 2015 14:07
@robertkowalski @Fishrock123
docs: delete unused/duplicate css files
1cb72c1 
 - `sh.css` already exists in `api_assets`
 - `sh_vim-dark.css` is unused, but used in the repo `node-website`
        now

Reviewed-by: Trevor Norris <[email protected]>
Signed-off-by: Julien Gilli <[email protected]>

PORT-FROM: joyent/node @ 0c50195
PR-URL: nodejs#1770
Reviewed-By: Roman Reiss <[email protected]>
Reviewed-By: Jeremiah Senkpiel <[email protected]>
@tyleranton @Fishrock123
docs: add return value for sync fs functions
a79dece 
Clarify that synchronous functions in fs with no return value return
undefined.

Specify that fs.openSync() returns an integer and fs.existsSync()
returns true or false.

Fixes: nodejs/node-v0.x-archive#9313

PR: nodejs/node-v0.x-archive#9359
Reviewed-By: Julien Gilli <[email protected]>

PORT-FROM: joyent/node @ 51fe319
PR-URL: nodejs#1770
Reviewed-By: Roman Reiss <[email protected]>
Reviewed-By: Jeremiah Senkpiel <[email protected]>

Conflicts:
	doc/api/fs.markdown
@Fishrock123 Fishrock123 merged commit a79dece into nodejs:master Jun 1, 2015
andrewdeandrade pushed a commit to andrewdeandrade/node that referenced this pull request Jun 3, 2015
@robertkowalski @andrewdeandrade
docs: delete unused/duplicate css files
359f234 
 - `sh.css` already exists in `api_assets`
 - `sh_vim-dark.css` is unused, but used in the repo `node-website`
        now

Reviewed-by: Trevor Norris <[email protected]>
Signed-off-by: Julien Gilli <[email protected]>

PORT-FROM: joyent/node @ 0c50195071a57bc40df82c8f57d341a435ff1eb6
PR-URL: nodejs/node#1770
Reviewed-By: Roman Reiss <[email protected]>
Reviewed-By: Jeremiah Senkpiel <[email protected]>
andrewdeandrade pushed a commit to andrewdeandrade/node that referenced this pull request Jun 3, 2015
@tyleranton @andrewdeandrade
docs: add return value for sync fs functions
68e38d8 
Clarify that synchronous functions in fs with no return value return
undefined.

Specify that fs.openSync() returns an integer and fs.existsSync()
returns true or false.

Fixes: nodejs/node-v0.x-archive#9313

PR: nodejs/node-v0.x-archive#9359
Reviewed-By: Julien Gilli <[email protected]>

PORT-FROM: joyent/node @ 51fe319faf4399fd027f8b32d1c425200b911e44
PR-URL: nodejs/node#1770
Reviewed-By: Roman Reiss <[email protected]>
Reviewed-By: Jeremiah Senkpiel <[email protected]>

Conflicts:
	doc/api/fs.markdown
@rvagg rvagg mentioned this pull request Jun 11, 2015
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
meta Issues and PRs related to the general management of the project.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
7 participants
@Fishrock123 @domenic @silverwind @bnoordhuis @mscdex @robertkowalski @tyleranton